Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix/window event race condition #842

Open
wants to merge 43 commits into
base: main
Choose a base branch
from
Open

Fix/window event race condition #842

wants to merge 43 commits into from

Conversation

Animesh404
Copy link
Contributor

@Animesh404 Animesh404 commented Jul 8, 2024 •
edited
Loading

What kind of change does this PR introduce?

a bugfix for #816

Summary

  • removed data from WindowEvent model and created a new model A11yEvent which points to the relevant WindowEvent
  • updated crud.py by adding insert_a11y_event
  • made changes in record.py where we use queue to map which WindowEvent is waiting for its corresponding A11yEvent

Checklist

  • My code follows the style guidelines of OpenAdapt
  • I have performed a self-review of my code
  • If applicable, I have added tests to prove my fix is functional/effective
  • I have linted my code locally prior to submission
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. README.md, requirements.txt)
  • New and existing unit tests pass locally with my changes

@abrichr abrichr mentioned this pull request Jul 10, 2024
Closed
abrichr
abrichr requested changes Jul 12, 2024
View reviewed changes
Copy link
Member

@abrichr abrichr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Animesh404 ! I left a few comments.

Can you please also test out https://github.com/Kalmat/PyWinCtl ? I believe it should also support MacOS.

openadapt/window/_windows.py Outdated Show resolved Hide resolved
openadapt/window/_windows.py Outdated Show resolved Hide resolved
openadapt/window/_windows.py Outdated Show resolved Hide resolved
openadapt/window/_windows.py Outdated Show resolved Hide resolved
openadapt/window/_windows.py Outdated Show resolved Hide resolved
openadapt/window/__init__.py Outdated Show resolved Hide resolved
openadapt/window/_windows.py Outdated Show resolved Hide resolved
@Animesh404 Animesh404 marked this pull request as draft July 16, 2024 21:29
@Animesh404 Animesh404 marked this pull request as draft July 16, 2024 21:29
abrichr
abrichr reviewed Jul 17, 2024
View reviewed changes
openadapt/models.py Outdated Show resolved Hide resolved
abrichr
abrichr reviewed Jul 17, 2024
View reviewed changes
openadapt/record.py Outdated Show resolved Hide resolved
abrichr
abrichr reviewed Jul 17, 2024
View reviewed changes
openadapt/record.py Outdated Show resolved Hide resolved
@abrichr
Copy link
Member

abrichr commented Jul 18, 2024

@Animesh404 please consolidate the alembic scripts into a single migration. You can do this by downgrading the two that you have currently applied:

alembic downgrade -2

Then generate a new revision, add it, and remove the existing ones.

abrichr
abrichr reviewed Jul 22, 2024
View reviewed changes
openadapt/models.py Outdated Show resolved Hide resolved
abrichr
abrichr reviewed Jul 22, 2024
View reviewed changes
openadapt/models.py Outdated Show resolved Hide resolved
abrichr
abrichr reviewed Jul 22, 2024
View reviewed changes
openadapt/models.py Outdated Show resolved Hide resolved
abrichr
abrichr reviewed Jul 23, 2024
View reviewed changes
openadapt/record.py Outdated Show resolved Hide resolved
abrichr
abrichr reviewed Jul 23, 2024
View reviewed changes
openadapt/record.py Outdated Show resolved Hide resolved
abrichr
abrichr reviewed Jul 23, 2024
View reviewed changes
openadapt/record.py Outdated Show resolved Hide resolved
abrichr
abrichr reviewed Jul 24, 2024
View reviewed changes
openadapt/models.py Outdated Show resolved Hide resolved
abrichr
abrichr reviewed Jul 24, 2024
View reviewed changes
openadapt/models.py Outdated Show resolved Hide resolved
@Animesh404 Animesh404 marked this pull request as draft July 25, 2024 11:52
@Animesh404 Animesh404 marked this pull request as ready for review July 25, 2024 17:11
@abrichr
Copy link
Member

abrichr commented Jul 25, 2024

Thanks @Animesh404 ! Can you please show screenshots of a11y events in visualize.py and the dashboard?

@Animesh404
Copy link
Contributor Author

Thanks @Animesh404 ! Can you please show screenshots of a11y events in visualize.py and the dashboard?

Sure @abrichr

  • Dashboard
    image
  • Visualize
    image

@abrichr
Copy link
Member

abrichr commented Jul 25, 2024

@Animesh404 please correct me if I'm wrong but I don't see any accessibility data in the dashboard screenshot

@Animesh404 Animesh404 marked this pull request as draft July 26, 2024 08:58
@Animesh404
Copy link
Contributor Author

@Animesh404 please correct me if I'm wrong but I don't see any accessibility data in the dashboard screenshot

image
added here 3b0ada0

@Animesh404 Animesh404 marked this pull request as ready for review July 29, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abrichr abrichr abrichr requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Animesh404 @abrichr